-
Notifications
You must be signed in to change notification settings - Fork 212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding regex configuration to the parameter-name-style rule #2213
Adding regex configuration to the parameter-name-style rule #2213
Conversation
43a5479
to
d3a0a57
Compare
d3a0a57
to
f9359f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to make sure to be able to use the old configuration value with the named values.
configuration, | ||
{{"localparam_style", SetNamedBits(&localparam_allowed_style_, choices)}, | ||
{"parameter_style", SetNamedBits(¶meter_allowed_style_, choices)}}); | ||
{{"localparam_style_regex", SetRegex(&localparam_style_regex_)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't remove the old configuration names: if someone has used localparam_style and parameter_style in their configuration it will suddenly fail as it doesn't exist anymore.
Instead, we should have not only the new regex names, but also the old names, which then select a regex.
So someone can set localparam_style_regex
, but they can also say localparam_style=ALL_CAPS
, which could be considered a name for a regex; So if such value has been set, we then then can set the upper case regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a preference on how it should handle if both localparam_style
and localparam_style_regex
are set with valid values?
Maybe have ParameterNameStyleRule::Configure
return an absl::InvalidArgumentError()
noting the user should use one or the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, maybe ignore localparam_style_regex
unless the user sets localparam_style
to some value (say "regex") so the user has to explicitly enable that feature.
Just looking at the code again, localparam_style
is an OR'd list of things, so when the rule is configured it would have OR multiple regex's together, potentially including the users regex, simple enough.
Anyway, let me know if you have a preference (or better solution).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, the OR-ing part, I didn't think of it: yes that should then assemble a (foo)|(bar)
kindof regex.
But with OR-ing in place, we can assemble all the regex from localparam_style
with the localparam_style_regex
if both of them are given in the lint configuration.
Probably good to have a test for all these situations; for that, you could have public functions that return the const RE2 *
for each regex
const RE2 *localparam_style_regex() const { return localparam_style_regex_.get(); }
, so that it is easy to write a test that verifies that the returned value is re != nullptr
and the re->pattern()
matches the expectation after having called Configuration()
.
const std::initializer_list<LintTestCase> kTestCases = { | ||
{"package a; parameter int FooBar = 1; endpackage"}, | ||
{"package a; parameter int ", {kToken, "FOO_BAR"}, " = 1; endpackage"}, | ||
{"module a; localparam int ", {kToken, "FooBar"}, " = 1; endmodule"}, | ||
{"module a; localparam int FOO_BAR = 1; endmodule"}, | ||
}; | ||
RunConfiguredLintTestCases<VerilogAnalyzer, ParameterNameStyleRule>( | ||
kTestCases, "parameter_style:CamelCase;localparam_style:ALL_CAPS"); | ||
kTestCases, | ||
"parameter_style_regex:([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?;localparam_" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example shows how hard it is to actually use if just the defaults would be needed; so making it possible to set the parameter_style:CamelCase
will not only be backwards compatible, but also much more usable while still keeping the possibility open to actually use the regex.
36e5bcb
to
03ad538
Compare
…. The rule now forms a single regex using the original configuration values and a user regex.
03ad538
to
23d76be
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #2213 +/- ##
========================================
Coverage 92.92% 92.92%
========================================
Files 359 360 +1
Lines 26740 26912 +172
========================================
+ Hits 24848 25008 +160
- Misses 1892 1904 +12 ☔ View full report in Codecov by Sentry. |
Ok, I think I've addressed all issues, with the regex configuration now extending the existing configuration in a non-breaking way. |
Oh, got distracted. Will do another review today. |
Thanks! Sorry for the delay, sometimes life is just busy... |
All good. |
Adding regex configuration to the parameter-name-style rule, part of issue #2074